-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
⚠ ClusterExtension: add installNamespace, remove watchNamespaces #810
⚠ ClusterExtension: add installNamespace, remove watchNamespaces #810
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
/hold I want to add a few more tests that are specific to the use of the new InstallNamespace field. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #810 +/- ##
==========================================
+ Coverage 64.64% 64.66% +0.01%
==========================================
Files 16 16
Lines 1321 1316 -5
==========================================
- Hits 854 851 -3
+ Misses 404 403 -1
+ Partials 63 62 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d89641d
to
16012c4
Compare
{"hypen-separated", "hyphenated-name", ""}, | ||
{"no install namespace", "", regexMismatchError}, | ||
{"dot-separated", "dotted.name", regexMismatchError}, | ||
{"includes version", "channel-has-version-1.0.1", regexMismatchError}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already check for dot separated, I don't think we need to explicitly check for "includes version" for install namespaces
{"includes version", "channel-has-version-1.0.1", regexMismatchError}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep good point, this was a copy/paste job from the channel name tests, and this one does not seem relevant here. I'll remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One minor nit, but IMO not something that is worth holding this PR on and will leave up to you if you'd like to address.
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
16012c4
to
9befb90
Compare
0f85c25
Description
In
ClusterExtension
:spec.installNamespace
as a required field. This will be used to configure the context of the client that templates and applies the bundle contents. This will also be necessary for the introduction of aspec.serviceAccount
field that operator-controller will use to perform all interactions with the apiserver for that ClusterExtension.spec.watchNamespaces
and hardcode all registry+v1 based bundles to watch all namespaces (or fail if they don't support AllNamespaces mode)Reviewer Checklist